suite: fix SetupSubTest and TearDownSubTest execution order #1471

linusbarth · 2023-09-15T10:34:46Z

Summary

The order of execution of SetupSubTest and TearDownSubTest methods was changed so that the testing context inside these methods is correct.

Changes

The SetupSubTest was moved to after the SetT that set the child testing context
The TearDownSubTest was moved to before the deferred SetT that reset the testing context (execution wise)
The TearDownSubTest and SetT were deferred in one block. This was fine beforehand because a panic in the TearDownSubTest would have had no influence on the "suite.SetT(oldT)". Now since a panic in the TearDownSubTest would lead to omitting the "suite.SetT(oldT)" this defer was split into two separate defers.

Motivation

There were two problems with the order of execution in the Suite.Run() method:

One could not access the correct testing context ("s.T()") inside the SetupSubTest and TearDownSubTest methods. If the testing context was used for e.g. assertions of mocks in the TearDownSubTest, the results would not be "attached" to the correct test in the test output.
The behavior was different to the order of execution for "root" tests regarding the SetupTest and TearDownTest methods (see lines 167-201). This could confuse users of the library.

Related issues

Closes #1465

dolmen · 2023-10-10T12:43:40Z

suite/suite.go

+			if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok {
+				tearDownSubTest.TearDownSubTest()
+			}
+		}()


Move the if outside of the defer:

if tearDownSubTest, ok := suite.s.(TearDownSubTest); ok { defer tearDownSubTest.TearDownSubTest() }

Done. Also I took the liberty to extract the defer-function in line 102. Please note if I should revert that. I think it should not produce noticeable change.

dolmen

We also need test that enforces the order. That test should fail before your fix and succeed after.

linusbarth · 2023-10-10T15:52:09Z

We also need test that enforces the order. That test should fail before your fix and succeed after.

done

dolmen · 2023-10-15T22:28:23Z

@linusbarth Please drop the last commit (merge commit). Use git rebase instead.

dolmen · 2023-10-15T22:42:37Z

suite/suite_test.go

@@ -259,10 +259,14 @@ func (suite *SuiteTester) TestSubtest() {

 func (suite *SuiteTester) TearDownSubTest() {
 	suite.TearDownSubTestRunCount++
+	// We should get the *testing.T for the test that is to be torn down


Move this comment as 3rd argument of Contains below.

dolmen · 2023-10-15T22:42:47Z

suite/suite_test.go

 }

 func (suite *SuiteTester) SetupSubTest() {
 	suite.SetupSubTestRunCount++
+	// We should get the *testing.T for the test that is to be set up


Move this comment as 3rd argument of Contains below.

There were two problems with the order of execution in the Suite.Run() method: - One could not access the correct testing context ("s.T()") inside the SetupSubTest and TearDownSubTest methods. If the testing context was used for e.g. assertions of mocks in the TearDownSubTest, the results would not be "attached" to the correct test in the test output. - The behavior was different to the order of execution for "root" tests (see lines 167-201) regarding the SetupTest and TearDownTest methods. This could confuse users of the library. Also the logic to be deferred was joined together. This was fine beforehand because a panic in the TearDownSubTest would have had no influence on the "suite.SetT(oldT)". Now since a panic in the TearDownSubTest would lead to omitting the "suite.SetT(oldT)" this defer was split into two separate defers.

tomhutch

Nice, I'd be really happy to see this fix get in.

tomhutch · 2023-10-16T17:05:56Z

suite/suite_test.go

@@ -259,10 +259,12 @@ func (suite *SuiteTester) TestSubtest() {

 func (suite *SuiteTester) TearDownSubTest() {
 	suite.TearDownSubTestRunCount++
+	suite.Contains(suite.T().Name(), "subtest", "We should get the *testing.T for the test that is to be torn down")


Updating the test suite attributes here and performing assertions alongside the others in the main test body will be more consistent with the current testing setup:

testify/suite/suite_test.go

Lines 154 to 172 in ac5cd69

SetupSuiteRunCount int

TearDownSuiteRunCount int

SetupTestRunCount int

TearDownTestRunCount int

TestOneRunCount int

TestTwoRunCount int

TestSubtestRunCount int

NonTestMethodRunCount int

SetupSubTestRunCount int

TearDownSubTestRunCount int

SuiteNameBefore []string

TestNameBefore []string

SuiteNameAfter []string

TestNameAfter []string

TimeBefore []time.Time

TimeAfter []time.Time

See this for reference:
https://github.com/stretchr/testify/pull/1393/files#diff-ca4ffdb7050ad570420ac5682fefad8481384709c2884051cd57071283ce9ca4

Thanks for your review! You're right, that is more consistent. I mostly just copied your referenced testing code now into my branch.

suite/suite.go

This fix adds panic handling for subtests which will achieve: - subtests will fail for the correct test context when panicking - the test execution is not stopped; the next subtest will be executed

dolmen added pkg-suite Change related to package testify/suite bug labels Oct 10, 2023

dolmen requested changes Oct 10, 2023

View reviewed changes

dolmen added the hacktoberfest-accepted Hacktoberfest label Oct 10, 2023

dolmen requested changes Oct 10, 2023

View reviewed changes

dolmen changed the title ~~fix: SetupSubTest and TearDownSubTest execution order~~ suite: fix SetupSubTest and TearDownSubTest execution order Oct 10, 2023

dolmen force-pushed the fix-setup-and-teardown-subtest-execution-order branch from 4f8dd94 to 7d0eaf9 Compare October 15, 2023 22:35

dolmen requested changes Oct 15, 2023

View reviewed changes

linusbarth force-pushed the fix-setup-and-teardown-subtest-execution-order branch 3 times, most recently from 33e63d9 to c46c72b Compare October 16, 2023 00:59

linusbarth added 5 commits October 16, 2023 03:04

improve: defer-style in Suite.Run()

83e44bc

test: call order of setup/teardown for subtests

4657972

test: testing.T correctness in subtests setup/teardown

aa01ed0

improve: move comment to msgAndArgs-param in test

ac5cd69

linusbarth force-pushed the fix-setup-and-teardown-subtest-execution-order branch from c46c72b to ac5cd69 Compare October 16, 2023 01:04

This was referenced Oct 16, 2023

suite: fix test failures #1421

Merged

Add flag -testify.shuffle for suites #1413

Open

Ensure that subtest name is set in SetupSubTest/TearDownSubTest #1393

Closed

thekidder approved these changes Oct 16, 2023

View reviewed changes

dolmen mentioned this pull request Oct 16, 2023

suite: recover and fail on subtest #1455

Closed

dolmen previously approved these changes Oct 16, 2023

View reviewed changes

tomhutch reviewed Oct 16, 2023

View reviewed changes

improve: tests for asserting test names in subtests

c8efd3c

linusbarth dismissed dolmen’s stale review via c8efd3c October 17, 2023 14:36

xwjdsh reviewed Oct 17, 2023

View reviewed changes

suite/suite.go Show resolved Hide resolved

fix: panic behavior for subtests

91478fe

This fix adds panic handling for subtests which will achieve: - subtests will fail for the correct test context when panicking - the test execution is not stopped; the next subtest will be executed

renovate bot mentioned this pull request Apr 3, 2024

Update all non-major dependencies bitrise-io/bitrise-workflow-editor#823

Merged

1 task

renovate bot mentioned this pull request Jul 22, 2024

fix(deps): update all moul/u#74

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suite: fix SetupSubTest and TearDownSubTest execution order #1471

suite: fix SetupSubTest and TearDownSubTest execution order #1471

linusbarth commented Sep 15, 2023

dolmen Oct 10, 2023 •

edited

Loading

linusbarth Oct 10, 2023

dolmen left a comment

linusbarth commented Oct 10, 2023

dolmen commented Oct 15, 2023

dolmen Oct 15, 2023

linusbarth Oct 16, 2023

dolmen Oct 15, 2023

linusbarth Oct 16, 2023

tomhutch left a comment

tomhutch Oct 16, 2023

linusbarth Oct 17, 2023 •

edited

Loading

	SetupSuiteRunCount int
	TearDownSuiteRunCount int
	SetupTestRunCount int
	TearDownTestRunCount int
	TestOneRunCount int
	TestTwoRunCount int
	TestSubtestRunCount int
	NonTestMethodRunCount int
	SetupSubTestRunCount int
	TearDownSubTestRunCount int

	SuiteNameBefore []string
	TestNameBefore []string

	SuiteNameAfter []string
	TestNameAfter []string

	TimeBefore []time.Time
	TimeAfter []time.Time

suite: fix SetupSubTest and TearDownSubTest execution order #1471

suite: fix SetupSubTest and TearDownSubTest execution order #1471

Conversation

linusbarth commented Sep 15, 2023

Summary

Changes

Motivation

Related issues

dolmen Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

linusbarth Oct 10, 2023

Choose a reason for hiding this comment

dolmen left a comment

Choose a reason for hiding this comment

linusbarth commented Oct 10, 2023

dolmen commented Oct 15, 2023

dolmen Oct 15, 2023

Choose a reason for hiding this comment

linusbarth Oct 16, 2023

Choose a reason for hiding this comment

dolmen Oct 15, 2023

Choose a reason for hiding this comment

linusbarth Oct 16, 2023

Choose a reason for hiding this comment

tomhutch left a comment

Choose a reason for hiding this comment

tomhutch Oct 16, 2023

Choose a reason for hiding this comment

linusbarth Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

dolmen Oct 10, 2023 •

edited

Loading

linusbarth Oct 17, 2023 •

edited

Loading